drivers: ethernet: vsc8541: various fixes #91726

robhancocksed · 2025-06-16T22:16:12Z

Added missing MDIO enable/disable calls around accessing PHY registers, to make sure the MDIO bus is active first
Cleaned up internal functions to use uint16_t for PHY register values
Fixed inverted reset GPIO line
Added SW reset timeout
Implemented cfg_link function required by some MAC drivers

robhancocksed · 2025-06-16T23:57:24Z

It looks like the arch.interrupt.gen_isr_table_local.riscv test is failing on the ganymed_bob and ganymed_sk boards where I modified the .dts files. However, those build errors also seem to happen on the main branch, so they seem unrelated to these changes?

maass-hamburg · 2025-06-17T08:31:11Z

boards/sensry/ganymed_bob/ganymed_bob_sy120_gbm.dts

@@ -30,7 +30,7 @@
 		reg = <0x0>;
 		status = "okay";

-		reset-gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio0 11 GPIO_ACTIVE_LOW>;


add a entry in the migration guide, stating that this got fixed, so users with custom boards know that they have to change it too

maass-hamburg · 2025-06-17T08:33:44Z

drivers/ethernet/phy/phy_microchip_vsc8541.c

 */
-static int phy_mc_vsc8541_cfg_link(const struct device *dev, enum phy_link_speed speeds)
+static int phy_mc_vsc8541_cfg_link(const struct device *dev, enum phy_link_speed adv_speeds)


maybe wait until #91354 is merged, it will provide phy_mii_cfg_link_autoneg() to set the autoneg registers according to the ethernet specs

check out #91572 62e692e

Rebased on top of those changes - we can now make use of some of the common phy_mii code here.

Feel free to cherry-pick b58b241 from #91572
and putting it before your changes

I fixed there some other things I noticed

maass-hamburg · 2025-06-17T08:42:21Z

drivers/ethernet/phy/phy_microchip_vsc8541.c

+		if (count++ > 1000) {
+			LOG_ERR("phy reset timed out");
+			return -ETIMEDOUT;
+		}


/* we use limited advertising, to force gigabit speed */ /* initial version of this driver supports only 1GB/s */ /* 1000MBit/s + AUTO */ ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_ADV, (1 << 8) | (1 << 6) | 0x01); if (ret) { return ret; } ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_CTRL1000, (1 << 12) | (1 << 11) | (1 << 9)); if (ret) { return ret; } /* start auto negotiation */ ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_BMCR, BMCR_ANENABLE | BMCR_ANRESTART); if (ret) { return ret; }

this can probably be removed, autoneg is enabled by default

maass-hamburg · 2025-06-18T06:49:36Z

drivers/ethernet/phy/phy_microchip_vsc8541.c

-	if (ret) {
-		return ret;
-	}
-
 	/* start auto negotiation */
 	ret = phy_mc_vsc8541_write(dev, PHY_REG_PAGE0_BMCR, BMCR_ANENABLE | BMCR_ANRESTART);


this linke is also no longer needed, as we don't change the autoneg regs on startup, so we don't need to restart it, autoneg is btw enabled by default

maass-hamburg · 2025-06-18T07:08:07Z

drivers/ethernet/phy/phy_microchip_vsc8541.c

+/**
+ * @brief Reconfigure the link speed
+ *
+ * @param dev device structure to phy
+ * @param adv_speeds speeds to be advertised
+ * @param flags configuration flags
+ */


these kind of descriptions for doxygen are only needed for public apis in their headers, not in the source files

maass-hamburg · 2025-06-18T10:14:19Z

drivers/ethernet/phy/phy_microchip_vsc8541.c

 */
-static int phy_mc_vsc8541_cfg_link(const struct device *dev, enum phy_link_speed speeds)
+static int phy_mc_vsc8541_cfg_link(const struct device *dev, enum phy_link_speed adv_speeds)


Feel free to cherry-pick b58b241 from #91572
and putting it before your changes

I fixed there some other things I noticed

robhancocksed · 2025-06-18T19:33:19Z

@maass-hamburg I cherry-picked in the commit you mentioned, as well as one other with a mutex change, from your branch and rebased my changes on top of it. I think I have addressed your other comments as well.

robhancocksed · 2025-06-18T20:29:28Z

As noted before, those ganymed_bob and ganymed_sk test failures are not new, they also are occurring on weekly builds on the main branch.

drivers/ethernet/phy/phy_microchip_vsc8541.c

- implement configure link - support half duplex - use defines from mii.h - fix check ret vals Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com> Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>

use mutex to protect page register phy_mc_vsc8541_get_link got removed from phy_mc_vsc8541_link_cb_set so, that phy_mc_vsc8541_link_monitor (own thread) is the only one to change data->state Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>

Fixed some build warnings in the driver from previous changes by removing an unused variable and hooking up the cfg_link function. Also removes some implicit boolean conversions. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

The driver was not enabling the MDIO bus before trying to access registers. Added enabling and disabling the bus around PHY register accesses. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

The internal register read/write functions used uint32_t for the values even though the registers are only 16 bits wide, resulting in a bunch of casting. Change the internal functions to use uint16_t and wrap them for the external read/write API which uses uint32_t. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

For GPIOs driving active-low signals, such as the VSC8541's reset pin, they are supposed to be declared as active low in the device tree, and set to 1 to assert and 0 to clear. Change the driver as such so that it does not leave the PHY stuck in reset when so configured. Also changed all in-tree board DTS files for this PHY to properly declare the reset GPIO as active low. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

Mention a change to the reset-gpios handling in the vsc8541 PHY driver. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

drivers/ethernet/phy/phy_microchip_vsc8541.c

The driver previously could enter an infinite loop if the PHY software reset failed to complete, which could happen due to hardware reset issues or MDIO bus problems. Add a timeout of 1000 iterations so we report an error in this scenario rather than causing a lockup. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

Add support for disabling autonegotiation to the cfg_link callback, as with the phy_mii driver. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

maass-hamburg · 2025-06-25T14:36:22Z

@robhancocksed please rebase to current main, hopefully the unrelated CI fails had been fixed, if not please open an issue

sonarqubecloud · 2025-06-25T15:08:41Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

maass-hamburg · 2025-06-25T16:15:57Z

needs #92194 to be fixed

robhancocksed · 2025-06-25T16:24:29Z

@robhancocksed please rebase to current main, hopefully the unrelated CI fails had been fixed, if not please open an issue

As you noted, the test failures on ganymed_bob are also occurring on the main branch, so rebasing will not help at this point. I assume this PR is flagging those failures since it touched the .dts files for those boards. So either this can be merged despite the test failures, or #92194 will need to be fixed.

maass-hamburg · 2025-06-25T16:27:13Z

@robhancocksed please rebase to current main, hopefully the unrelated CI fails had been fixed, if not please open an issue

As you noted, the test failures on ganymed_bob are also occurring on the main branch, so rebasing will not help at this point. I assume this PR is flagging those failures since it touched the .dts files for those boards. So either this can be merged despite the test failures, or #92194 will need to be fixed.

correct, probably needs a fix like #91391 but for your effected boards

dkalowsk · 2025-06-28T06:32:55Z

@robhancocksed RC1 has been tagged, which means all new PRs need to be bug fixes with an attached GitHub issue. Given that this PR is all bug fixes, can you create an issue and attach it to this PR so we can merge for RC2?

robhancocksed · 2025-07-02T16:30:43Z

@dkalowsk I created an issue entry to track the problems being fixed here: #92553

github-actions bot added area: Ethernet platform: sensry labels Jun 16, 2025

github-actions bot requested review from clamattia, decsny, lmajewski, maass-hamburg and pdgendt June 16, 2025 22:17

github-actions bot assigned maass-hamburg Jun 16, 2025

maass-hamburg requested changes Jun 17, 2025

View reviewed changes

robhancocksed force-pushed the vsc8541-fixes branch from b30a0e3 to 6dcae80 Compare June 17, 2025 17:49

github-actions bot added the Release Notes To be mentioned in the release notes label Jun 17, 2025

github-actions bot requested review from danieldegrasse, dkalowsk and kartben June 17, 2025 17:50

robhancocksed requested a review from maass-hamburg June 17, 2025 18:48

maass-hamburg requested changes Jun 18, 2025

View reviewed changes

robhancocksed force-pushed the vsc8541-fixes branch from 6dcae80 to 512218d Compare June 18, 2025 19:27

maass-hamburg previously approved these changes Jun 18, 2025

View reviewed changes

pdgendt reviewed Jun 19, 2025

View reviewed changes

drivers/ethernet/phy/phy_microchip_vsc8541.c Outdated Show resolved Hide resolved

maass-hamburg and others added 7 commits June 19, 2025 15:54

drivers: ethernet: phy: microchip_vsc8541: improve driver

b977f87

- implement configure link - support half duplex - use defines from mii.h - fix check ret vals Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com> Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>

drivers: ethernet: phy: vsc8541: add MDIO enable/disable

3ed64ec

The driver was not enabling the MDIO bus before trying to access registers. Added enabling and disabling the bus around PHY register accesses. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

doc: migration guide: ethernet: Mention vsc8541 reset changes

ad67407

Mention a change to the reset-gpios handling in the vsc8541 PHY driver. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

robhancocksed dismissed maass-hamburg’s stale review via dabca3a June 19, 2025 22:10

robhancocksed force-pushed the vsc8541-fixes branch from 512218d to dabca3a Compare June 19, 2025 22:10

robhancocksed requested review from pdgendt and maass-hamburg June 19, 2025 22:23

maass-hamburg reviewed Jun 23, 2025

View reviewed changes

drivers/ethernet/phy/phy_microchip_vsc8541.c Outdated Show resolved Hide resolved

robhancocksed added 2 commits June 24, 2025 13:03

drivers: ethernet: phy: vsc8541: allow disabling autonegotiation

bdecf2d

Add support for disabling autonegotiation to the cfg_link callback, as with the phy_mii driver. Signed-off-by: Robert Hancock <robert.hancock@calian.com>

robhancocksed force-pushed the vsc8541-fixes branch from dabca3a to bdecf2d Compare June 24, 2025 19:10

robhancocksed requested a review from maass-hamburg June 24, 2025 19:10

maass-hamburg approved these changes Jun 24, 2025

View reviewed changes

pdgendt approved these changes Jun 25, 2025

View reviewed changes

kartben closed this Jun 25, 2025

kartben reopened this Jun 25, 2025

maass-hamburg mentioned this pull request Jun 25, 2025

arch: riscv: arch.interrupt.gen_isr_table_local.riscv fails on multiple platforms #92194

Open

1 task

robhancocksed mentioned this pull request Jul 2, 2025

vsc8541 Ethernet PHY driver issues #92553

Open

1 task

maass-hamburg linked an issue Jul 2, 2025 that may be closed by this pull request

vsc8541 Ethernet PHY driver issues #92553

Open

1 task

dkalowsk added this to the v4.2.0 milestone Jul 5, 2025

maass-hamburg mentioned this pull request Jul 7, 2025

zephyr: sw_isr_table: Fix compilation error #92336

Open

drivers: ethernet: vsc8541: various fixes #91726

Are you sure you want to change the base?

drivers: ethernet: vsc8541: various fixes #91726

Conversation

robhancocksed commented Jun 16, 2025

Uh oh!

robhancocksed commented Jun 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robhancocksed commented Jun 18, 2025

Uh oh!

robhancocksed commented Jun 18, 2025

Uh oh!

Uh oh!

Uh oh!

maass-hamburg commented Jun 25, 2025

Uh oh!

sonarqubecloud bot commented Jun 25, 2025

Quality Gate passed

Uh oh!

maass-hamburg commented Jun 25, 2025

Uh oh!

robhancocksed commented Jun 25, 2025

Uh oh!

maass-hamburg commented Jun 25, 2025

Uh oh!

dkalowsk commented Jun 28, 2025

Uh oh!

robhancocksed commented Jul 2, 2025

Uh oh!

Uh oh!